Skip to content

feat: tracing #237

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: tracing #237

wants to merge 1 commit into from

Conversation

@kayra1
Copy link
Contributor

@kayra1 kayra1 commented May 14, 2025 •
edited
Loading

Description

This PR adds tracing to notary and adds jaeger to the makefile deployment for testing tracing. This PR only includes adding a span in the middleware for the HTTP layer. It should be developed in the future to include traces in other internal packages.

Screenshot From 2025-05-21 10-38-14

Screenshot From 2025-05-21 10-38-31

Screenshot From 2025-05-21 10-39-23-1

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that validate the behaviour of the software
  • I validated that new and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@kayra1 kayra1 force-pushed the tracing branch 3 times, most recently from b0630fe to 6d0b52e Compare October 8, 2025 11:09
@kayra1 kayra1 marked this pull request as ready for review October 8, 2025 12:07
@kayra1 kayra1 requested a review from a team as a code owner October 8, 2025 12:07
@kayra1 kayra1 requested a review from shipperizer October 8, 2025 12:07
shipperizer
shipperizer reviewed Oct 9, 2025
View reviewed changes
Copy link
Collaborator

@shipperizer shipperizer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

plan for traces is to have each meaningful functions/methods to have a ctx and use that to start a span in each

this requires quite a lot of refactoring i'm afraid

@kayra1
Copy link
Contributor Author

kayra1 commented Oct 10, 2025

plan for traces is to have each meaningful functions/methods to have a ctx and use that to start a span in each
this requires quite a lot of refactoring i'm afraid

This PR doesn't enable traces except in one location. It is simply the skeleton required to add traces to anywhere in Notary. I opened it like this because I think it's sufficiently large and self contained.

Do you mean this PR requires refactoring or the way we pass ctx in Notary? If it's the latter, I agree with that, but I think this PR is not the place to do that.

@kayra1 kayra1 requested a review from saltiyazan October 10, 2025 18:40
@shipperizer
Copy link
Collaborator

plan for traces is to have each meaningful functions/methods to have a ctx and use that to start a span in each
this requires quite a lot of refactoring i'm afraid

This PR doesn't enable traces except in one location. It is simply the skeleton required to add traces to anywhere in Notary. I opened it like this because I think it's sufficiently large and self contained.

Do you mean this PR requires refactoring or the way we pass ctx in Notary? If it's the latter, I agree with that, but I think this PR is not the place to do that.

happy to have this merged and keep going on a separate pr

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@shipperizer shipperizer shipperizer approved these changes

@saltiyazan saltiyazan Awaiting requested review from saltiyazan

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@kayra1 @shipperizer